feat: 웹, 어드민 간 refresh token 분리#732
Conversation
- redis에 ADMIN_REFRESH:{userId} 형태로 토큰 저장
- 관련 테스트 구현
Walkthrough이 PR은 관리자 인증 시스템을 구현합니다. 1) 도메인 모델 추가 — Estimated code review effort🎯 4 (Complex) | ⏱️ ~45분 Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PMD (7.25.0)src/main/java/com/example/solidconnection/common/exception/ErrorCode.javaNo java executable found in PATH Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 563c0f1f94
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/example/solidconnection/auth/token/config/TokenProperties.java (1)
33-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick win1)
adminRefresh누락 시Map.of에서 즉시 실패하므로, 명시적 검증으로 원인을 드러내 주세요.Line 34-38 구간은 설정 누락 시 NPE로 터져 원인 파악이 어렵습니다.
Objects.requireNonNull(또는 명시적IllegalStateException)로 키 이름을 포함해 fail-fast 처리하는 편이 안전합니다.수정 예시
+import java.util.Objects; ... `@PostConstruct` public void init() { tokenConfigs = Map.of( - AccessToken.class, access, - RefreshToken.class, refresh, - AdminRefreshToken.class, adminRefresh, - SignUpToken.class, signUp + AccessToken.class, Objects.requireNonNull(access, "token.access is required"), + RefreshToken.class, Objects.requireNonNull(refresh, "token.refresh is required"), + AdminRefreshToken.class, Objects.requireNonNull(adminRefresh, "token.admin-refresh is required"), + SignUpToken.class, Objects.requireNonNull(signUp, "token.sign-up is required") ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/example/solidconnection/auth/token/config/TokenProperties.java` around lines 33 - 39, In TokenProperties.init(), add an explicit null-check for adminRefresh (and other required token config fields if desired) before calling Map.of so missing configuration throws a clear, fail-fast error; use Objects.requireNonNull(adminRefresh, "adminRefresh must be configured") or throw new IllegalStateException with that message, then populate tokenConfigs = Map.of(...); reference the init() method, the adminRefresh field, tokenConfigs, and the Map.of call when making the change.
🧹 Nitpick comments (3)
src/main/java/com/example/solidconnection/admin/auth/service/AdminAuthService.java (1)
69-76: 🏗️ Heavy lift1) 관리자
reissue는 refresh token 회전(rotate)까지 포함하는 구성이 더 안전합니다.현재는 access token만 갱신되어, 탈취된 admin refresh token의 재사용 가능 시간이 그대로 남습니다. 관리자 경로는 회전 전략(새 refresh 발급 + 기존 refresh 폐기 + 쿠키 교체)으로 줄이는 것을 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/example/solidconnection/admin/auth/service/AdminAuthService.java` around lines 69 - 76, AdminAuthService.reissue currently only issues a new AccessToken but must implement refresh token rotation: after validating requestedAdminRefreshToken via authTokenProvider.isValidAdminRefreshToken and parsing the SiteUser with authTokenProvider.parseSiteUser, generate a new AccessToken and a new RefreshToken (e.g., authTokenProvider.generateRefreshToken(siteUser)), persist or record the new refresh token and revoke/invalidate the old one (e.g., authTokenProvider.revokeRefreshToken(requestedAdminRefreshToken) or authTokenProvider.rotateRefreshToken(old, new)) in an atomic operation, and return the new refresh token to the caller (or ensure AdminReissueResponse.from(newAccessToken) is extended to include the new refresh token so the controller can replace the admin cookie). Ensure you reference AdminAuthService.reissue, authTokenProvider.isValidAdminRefreshToken, authTokenProvider.parseSiteUser, authTokenProvider.generateAccessToken, authTokenProvider.generateRefreshToken/revoke/rotate, and AdminReissueResponse when implementing these changes.src/test/java/com/example/solidconnection/admin/auth/service/AdminAuthServiceTest.java (1)
93-121: ⚡ Quick win1) 분리 목표를 직접 보장하는 부정 케이스 테스트를 추가해 주세요.
현재는 “저장된/삭제된 어드민 리프레시 토큰”만 검증합니다. PR 목표가 웹/어드민 토큰 분리인 만큼,
RefreshToken(웹용)을adminAuthService.reissue(...)에 넣었을 때 실패하는 케이스를 추가하면 회귀를 강하게 막을 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/example/solidconnection/admin/auth/service/AdminAuthServiceTest.java` around lines 93 - 121, Add a negative test in AdminAuthServiceTest that ensures adminAuthService.reissue(...) rejects a web RefreshToken: generate and save a web RefreshToken (use authTokenProvider.generateAndSaveRefreshToken(...) or equivalent that returns a RefreshToken), then call adminAuthService.reissue(refreshToken.token()) and assert it throws AuthException with the ADMIN_REFRESH_TOKEN_EXPIRED message; this verifies that passing a RefreshToken (not AdminRefreshToken) to adminAuthService.reissue is rejected.src/main/java/com/example/solidconnection/security/config/SecurityConfiguration.java (1)
66-68: ⚡ Quick win
/admin/auth/**전체 공개 대신 필요한 엔드포인트만 공개해 주세요.
- Line 66 설정은
sign-out까지 익명 접근 경로에 포함합니다. 현재도 컨트롤러에서 실패 처리되지만, 인증 경계는 보안 설정 레이어에서 먼저 닫는 편이 안전합니다.권한 범위 최소화 제안
- .requestMatchers("/admin/auth/**").permitAll() + .requestMatchers("/admin/auth/sign-in", "/admin/auth/reissue").permitAll() .requestMatchers("/admin/**").hasRole(ADMIN.name())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/example/solidconnection/security/config/SecurityConfiguration.java` around lines 66 - 68, The broad permitAll for "/admin/auth/**" in SecurityConfiguration should be narrowed: replace the wildcard with explicit public auth endpoints (e.g. "/admin/auth/sign-in", "/admin/auth/sign-up", "/admin/auth/password-reset" — but NOT the sign-out endpoint) inside the same HttpSecurity configuration method so anonymous access is only granted to those specific handlers; ensure the "/admin/**" matcher with hasRole(ADMIN.name()) remains and that the ordering of requestMatchers keeps admin role checks intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/example/solidconnection/auth/service/AuthTokenProvider.java`:
- Around line 80-85: Wrap the call to tokenProvider.parseSubject in
isValidAdminRefreshToken with a try-catch that catches parsing exceptions (e.g.,
IllegalArgumentException or the specific exception thrown by parseSubject) and
returns false on error; if parsing succeeds, proceed to use
tokenStorage.findToken(subject, AdminRefreshToken.class).map(...).orElse(false)
as before so any parse failure is normalized to false without propagating
exceptions.
In `@src/main/resources/config/application-variable.yml`:
- Around line 169-171: The admin refresh cookie is being set as secure
unconditionally which prevents browsers from saving it on http://localhost;
update AdminRefreshTokenCookieManager to set secure=false when running in
local/non-HTTPS environments or when the configured admin-refresh.cookie-domain
equals "localhost" (or is empty), and only set secure=true for HTTPS domains;
alternatively adjust application-variable.yml for local profile to use an empty
cookie-domain or a local-specific profile value and ensure the cookie builder
logic checks the domain/profile before applying secure(true).
In `@src/test/resources/application.yml`:
- Around line 95-98: The test YAML for token.admin-refresh is missing the
cookie-name required by the AdminRefreshTokenCookieProperties contract; update
the test configuration to include cookie-name (e.g., the same value used in
production) under token.admin-refresh so the AdminRefreshTokenCookieProperties
(cookie-name/cookie-domain) contract is satisfied and cookie creation in the
code paths that use AdminRefreshTokenCookieProperties will not receive a null
name.
---
Outside diff comments:
In
`@src/main/java/com/example/solidconnection/auth/token/config/TokenProperties.java`:
- Around line 33-39: In TokenProperties.init(), add an explicit null-check for
adminRefresh (and other required token config fields if desired) before calling
Map.of so missing configuration throws a clear, fail-fast error; use
Objects.requireNonNull(adminRefresh, "adminRefresh must be configured") or throw
new IllegalStateException with that message, then populate tokenConfigs =
Map.of(...); reference the init() method, the adminRefresh field, tokenConfigs,
and the Map.of call when making the change.
---
Nitpick comments:
In
`@src/main/java/com/example/solidconnection/admin/auth/service/AdminAuthService.java`:
- Around line 69-76: AdminAuthService.reissue currently only issues a new
AccessToken but must implement refresh token rotation: after validating
requestedAdminRefreshToken via authTokenProvider.isValidAdminRefreshToken and
parsing the SiteUser with authTokenProvider.parseSiteUser, generate a new
AccessToken and a new RefreshToken (e.g.,
authTokenProvider.generateRefreshToken(siteUser)), persist or record the new
refresh token and revoke/invalidate the old one (e.g.,
authTokenProvider.revokeRefreshToken(requestedAdminRefreshToken) or
authTokenProvider.rotateRefreshToken(old, new)) in an atomic operation, and
return the new refresh token to the caller (or ensure
AdminReissueResponse.from(newAccessToken) is extended to include the new refresh
token so the controller can replace the admin cookie). Ensure you reference
AdminAuthService.reissue, authTokenProvider.isValidAdminRefreshToken,
authTokenProvider.parseSiteUser, authTokenProvider.generateAccessToken,
authTokenProvider.generateRefreshToken/revoke/rotate, and AdminReissueResponse
when implementing these changes.
In
`@src/main/java/com/example/solidconnection/security/config/SecurityConfiguration.java`:
- Around line 66-68: The broad permitAll for "/admin/auth/**" in
SecurityConfiguration should be narrowed: replace the wildcard with explicit
public auth endpoints (e.g. "/admin/auth/sign-in", "/admin/auth/sign-up",
"/admin/auth/password-reset" — but NOT the sign-out endpoint) inside the same
HttpSecurity configuration method so anonymous access is only granted to those
specific handlers; ensure the "/admin/**" matcher with hasRole(ADMIN.name())
remains and that the ordering of requestMatchers keeps admin role checks intact.
In
`@src/test/java/com/example/solidconnection/admin/auth/service/AdminAuthServiceTest.java`:
- Around line 93-121: Add a negative test in AdminAuthServiceTest that ensures
adminAuthService.reissue(...) rejects a web RefreshToken: generate and save a
web RefreshToken (use authTokenProvider.generateAndSaveRefreshToken(...) or
equivalent that returns a RefreshToken), then call
adminAuthService.reissue(refreshToken.token()) and assert it throws
AuthException with the ADMIN_REFRESH_TOKEN_EXPIRED message; this verifies that
passing a RefreshToken (not AdminRefreshToken) to adminAuthService.reissue is
rejected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 76e10ccb-2b72-4aa3-bcec-dbed0059c206
📒 Files selected for processing (17)
src/main/java/com/example/solidconnection/admin/auth/controller/AdminAuthController.javasrc/main/java/com/example/solidconnection/admin/auth/controller/AdminRefreshTokenCookieManager.javasrc/main/java/com/example/solidconnection/admin/auth/controller/config/AdminRefreshTokenCookieProperties.javasrc/main/java/com/example/solidconnection/admin/auth/dto/AdminReissueResponse.javasrc/main/java/com/example/solidconnection/admin/auth/dto/AdminSignInRequest.javasrc/main/java/com/example/solidconnection/admin/auth/dto/AdminSignInResponse.javasrc/main/java/com/example/solidconnection/admin/auth/dto/AdminSignInResult.javasrc/main/java/com/example/solidconnection/admin/auth/service/AdminAuthService.javasrc/main/java/com/example/solidconnection/auth/domain/AdminRefreshToken.javasrc/main/java/com/example/solidconnection/auth/service/AuthTokenProvider.javasrc/main/java/com/example/solidconnection/auth/token/config/TokenProperties.javasrc/main/java/com/example/solidconnection/common/exception/CustomExceptionHandler.javasrc/main/java/com/example/solidconnection/common/exception/ErrorCode.javasrc/main/java/com/example/solidconnection/security/config/SecurityConfiguration.javasrc/main/resources/config/application-variable.ymlsrc/test/java/com/example/solidconnection/admin/auth/service/AdminAuthServiceTest.javasrc/test/resources/application.yml
| .path(PATH) | ||
| .maxAge(maxAge) | ||
| .domain(properties.cookieDomain()) | ||
| .sameSite(SameSite.LAX.attributeValue()) |
There was a problem hiding this comment.
기존 LAX 방식이었던 이유가 어떤 거였는지 알려주실 수 있나요?? 인증 정보가 관리지 웹(www.admins.solid-connection.com)과 서비스 웹(www.solid-connection.com)으로 완전 분리되었으니까 Strict를 해도 되지 않을까라는 생각이 들어 의견을 듣고 싶습니다!
There was a problem hiding this comment.
저 합류 이전이라 근거는 잘 모르겠습니다.
그리고 현재 쿠키 도메인이 .solid-connection.com 로 설정되어 있어, Strict을 사용해도 쿠키를 분리하는 효과를 기대하긴 어려울 거 같습니다 ..!
관련 이슈
작업 내용
ADMIN_REFRESH:{userId}형태로 토큰 저장됩니다.특이 사항
리뷰 요구사항 (선택)